Add ability to use different TSIG algorithms #626

bodgit · 2018-01-10T20:36:21Z

This makes it possible to fix #549 and support GSS-TSIG (or in theory any other TSIG algorithms aside from HMAC-MD5, etc.). There is one breaking change in that the TsigSecret field is now a map of value interface{} rather than string. I've added two new functions TsigGenerateByAlgorithm() and TsigVerifyByAlgorithm() which allow an algorithm-specific callback and refactored the existing TsigGenerate() and TsigVerify() to just call these with callback functions that perform the HMAC-specific bits.

With this, GSS-TSIG can be done with something like the following:

const (
        GssTsig = "gss-tsig."
)

func main() {
        ...

        ctx, keyname, err := negotiateGssapiCtx(hostname)
        if err != nil {
                ...
        }

        client := &dns.Client{
                Net:           "tcp",
                TsigAlgorithm: map[string]*dns.TsigAlgorithm{GssTsig: {TsigGenerateGssapi, TsigVerifyGssapi}},
                TsigSecret:    map[string]interface{}{*keyname: ctx},
        }

        msg := new(dns.Msg)

        // Set dynamic DNS, etc.

        msg.SetTsig(*keyname, GssTsig, 300, time.Now().Unix())

        rr, _, err := client.Exchange(msg, net.JoinHostPort(addr, "53"))

        ...
}

negotiateGssapiCtx does the TKEY exchange to establish the GSSAPI context and my TsigGenerateGssapi & TsigVerifyGssapi are pretty much the same as in #549 (comment) the only difference is that the generate callback is now also passed the TSIG algorithm.

codecov-io · 2018-01-10T20:41:10Z

Codecov Report

Merging #626 into master will decrease coverage by 0.16%.
The diff coverage is 28.04%.

@@            Coverage Diff             @@
##           master     #626      +/-   ##
==========================================
- Coverage   58.23%   58.07%   -0.17%     
==========================================
  Files          37       37              
  Lines       10122    10161      +39     
==========================================
+ Hits         5895     5901       +6     
- Misses       3171     3202      +31     
- Partials     1056     1058       +2

Impacted Files	Coverage Δ
server.go	`59.75% <0%> (-2.33%)`	⬇️
client.go	`56.38% <5.26%> (-2.06%)`	⬇️
tsig.go	`43.49% <55%> (+1.98%)`	⬆️
msg.go	`79.34% <0%> (-0.63%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 350cd08...2450047. Read the comment docs.

tmthrgd · 2018-01-10T23:06:23Z

@bodgit I doubt such a breaking change would be accepted into the library. I see no reason why the meta argument, and thus this breaking change, is needed at all. What's stopping you from just using a closure? You can just pass co.TsigSecret[t.Hdr.Name] (string) and t.Hdr.Name into TsigGenerateByAlgorithm and TsigVerifyByAlgorithm instead, and have them passed into cb.

bodgit · 2018-01-11T09:07:48Z

The problem is that co.TsigSecret[t.Hdr.Name] isn't always a string, it is for traditional HMAC TSIG but for GSS-TSIG it's a per-server GSSAPI context which in this case is some cgo-bound pointer into a Kerberos C library and I didn't want to have to add any GSSAPI library-specific types to this library. The actual algorithm-specific function is the only area that needs to know how to interpret the secret, hence making it an interface{}.

I agree this breaks the library (although all libraries get a major version bump eventually) so I could add another field such as client.TsigAlgorithmSecret and keep the original client.TsigSecret as-is which keeps the library working exactly the same as before (I've not changed the prototype of any existing functions here), but then you have two fields that kind of look the same although if/when you do that major version bump you can rationalise them.

miekg · 2018-01-11T11:12:24Z

before I actually review +1 to @tmthrgd 's comment.

Even if it's a "cgo pointer" you can probably get away with it being converted to string (however bad that is).

miekg · 2018-01-11T11:14:02Z

Also note: we try hard to not break backwards compat, even if that puts us in a bad state code wise.

library versioning is not a reason to start breaking compat now

tmthrgd · 2018-01-11T13:34:36Z

I opened bodgit#1 to demonstrate my suggestion:

Restructure pull-request to avoid breaking the API

This commit removes the interface{} 'meta' argument, which required
breaking the signature of TsigSecret, and instead passes the name
directly to the callback. If there is a need to use a non-string secret,
this can be looked up from within a closure using the name as a lookup
key.

This change still permits all the same use-cases as it's parent.

I make no particular comment on this pull-request (#626) or whether it should be merged, but it is definitely possible to achieve the desired aim without introducing any breaking changes.

tmthrgd · 2018-01-11T13:37:34Z

master...tmthrgd:gss-tsig would be the final diff if @bodgit accepts my suggestion.

bodgit · 2018-01-11T16:13:54Z

Even if it's a "cgo pointer" you can probably get away with it being converted to string (however bad that is).

If that's possible, then that could work. When I print my GSSAPI context out with "%#v" I get this:

&gssapi.CtxId{Lib:(*gssapi.Lib)(0xc42009a000), C_gss_ctx_id_t:(gssapi._Ctype_gss_ctx_id_t)(0xff8370)}

I'm struggling to work out how to serialize that to and from a string.

bodgit · 2018-01-13T23:20:35Z

I've merged @tmthrgd's PR into mine, so there should now be no compat breakage.

tmthrgd · 2018-01-14T00:26:31Z

A few other nits:

Are TsigVerifyByAlgorithm and TsigGenerateByAlgorithm called by code external to the library? If not, they should be unexported.

In bodgit#1 you mention:

The only gotcha is I have to set a dummy TSIG secret in the client so it doesn't just return dns.ErrSecret.

Perhaps the if _, ok := co.TsigSecret[t.Hdr.Name]; !ok should only exist in the HMAC TsigVerify verify case? (Are there valid cases for an empty-string "" secret?)

tmthrgd · 2018-01-14T00:30:29Z

Also, does this functionality also need to exist in server.go for completeness?

See

dns/server.go

Line 559 in f5ac34d

w.tsigStatus = TsigVerify(m, w.tsigSecret[secret], "", false)

and

dns/server.go

Line 647 in f5ac34d

    
           data, w.tsigRequestMAC, err = TsigGenerate(m, w.tsigSecret[t.Hdr.Name], w.tsigRequestMAC, w.tsigTimersOnly)

bodgit · 2018-01-14T22:32:32Z

Are TsigVerifyByAlgorithm and TsigGenerateByAlgorithm called by code external to the library? If not, they should be unexported.

I've had a need to be able verify externally outside of the library; part of the TKEY negotiation includes a TSIG in the final response from the server which needs to be checked after the library would normally do it as the GSSAPI context needs to first be updated with the contents from that same TKEY response.

Generate less so, but maybe someone else has a use for that. The existing TsigVerify and TsigGenerate were exported so I made these new ones the same.

Perhaps the if _, ok := co.TsigSecret[t.Hdr.Name]; !ok should only exist in the HMAC TsigVerify verify case? (Are there valid cases for an empty-string "" secret?)

I don't think there are. Removing the check will mean an algorithm callback will always be passed at least an empty string so the onus is on that code to check that len(secret) > 0 or something similar. I'm completely ignoring that secret parameter in my GSS-TSIG callbacks anyway so I don't mind either way, whatever you think is best.

I will have a look at the server code now, I admit I overlooked it.

miekg · 2018-01-27T14:11:45Z

I'll to free up some cycles to review this - lots of other dns work pushed this backward.

bodgit · 2018-02-27T10:03:26Z

Anything I can do to progress this?

miekg · 2018-02-28T01:35:32Z

Sorry, just coming back to this after much work on CoreDNS.

Why do we need *TsigAlgorithm? That's still a breaking change...

I though in the long discussion we have about this that we could get away with a non-breaking backwards compat change and just having some closures?

I also (apparently) made it not clear enough that backward breaking changes for just this feature are not going to happen. I.e. new members in Client and Server can work, but changing existing fields and types will not fly.

bodgit · 2018-02-28T10:05:12Z

Can you show me where I've broken backwards compat? I don't believe I have (now). Originally I did by changing the tsigSecret map but I merged in @tmthrgd's PR which reverted that and made it work another way so all I've done is add the tsigAlgorithm member (which itself uses *TsigAlgorithm) which didn't exist previously.

bodgit · 2018-03-16T16:48:26Z

@miekg, can you possibly confer with @tmthrgd as I think he's happy there's no compat breakage. Other than that, I'd just like to know what to do with the server code I pointed out and if I should relax the secret check as suggested in #626 (comment)

This commit removes the interface{} 'meta' argument, which required breaking the signature of TsigSecret, and instead passes the name directly to the callback. If there is a need to use a non-string secret, this can be looked up from within a closure using the name as a lookup key. This change still permits all the same use-cases as it's parent.

bodgit · 2019-01-04T09:14:59Z

Can this be reopened? I'd still like to get this functionality in and @tmthrgd seemed happy with it. I'm not sure how many times I could point out that I'm only adding new members and not changing existing ones.

miekg · 2019-01-04T09:19:08Z

Sure. Didn't recall we where still discussing this...

…

On Fri, 4 Jan 2019, 09:15 Matt Dainty ***@***.*** wrote: Can this be reopened? I'd still like to get this functionality in and @tmthrgd <https://github.com/tmthrgd> seemed happy with it. I'm not sure how many times I could point out that I'm only adding new members and not changing existing ones. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#626 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVkWyxbB53_GLw-W1AcCnnbO8d58WXsks5u_xuUgaJpZM4RZ6I_> .

bodgit · 2019-01-04T09:51:05Z

I'm trying to but you keep ignoring my requests for feedback 🤷‍♂️

I'm happy to bend this into whatever shape is necessary so it works, I'm not changing any existing members or types, only adding new ones. So the only other question is regarding using closures, can you give me an example of how you think that would work that's better than how I've currently done it with callbacks?

DevKyleS · 2019-10-01T00:57:15Z

@miekg Can this get reopened? Would like to get GSS-TSIG support

miekg · 2019-10-03T09:48:35Z

[ Quoting <[email protected]> in "Re: [miekg/dns] Add ability to use ..." ]

Can this get reopened? Would like to get GSS-TSIG support

yes, sure, with these caveats: * NO BACKWARDS INCOMPATIBLE changes * NO BLOATING of current datastructures to accommodate the changes Once you have a PR that does that, we can discuss further. If not, then no

bodgit · 2019-10-03T15:14:32Z

NO BACKWARDS INCOMPATIBLE changes

I've asked you repeatedly to show me where such changes were in the latest version of the code, not in the first commits which were subsequently fixed thanks to @tmthrgd.

NO BLOATING of current datastructures to accommodate the changes

Quoting yourself:

I.e. new members in Client and Server can work, but changing existing fields and types will not fly.

So can I add new members, or is that bloating things? Please make your mind up.

The problem is the generation and verification of TSIG is done inside the library so there needs to be some way of being able to inject additional TSIG algorithms because as it currently stands if the algorithm used is not one of MD5, SHA1, SHA256 or SHA512 the library will always return ErrKeyAlg which breaks the standard usage of the client. In case it's not perfectly clear, GSS-TSIG does not use one of those algorithms.

miekg · 2019-10-03T15:36:15Z

I'm done with this feature. Sorry.

tmthrgd mentioned this pull request Jan 11, 2018

Restructure pull-request to avoid breaking the API bodgit/dns#1

Merged

bodgit force-pushed the gss-tsig branch from 1495e5f to 26c3980 Compare February 27, 2018 11:32

bodgit mentioned this pull request Mar 2, 2018

Support for GSSAPI/Kerberos signed updates hashicorp/terraform-provider-dns#30

Merged

bodgit and others added 4 commits May 22, 2018 21:08

Add ability to use different TSIG algorithms

9f2f210

Remove redundant string type in tsigGenerateHmac

2130008

Add custom TSIG algorithms to server too

2450047

bodgit force-pushed the gss-tsig branch from 26c3980 to 2450047 Compare May 22, 2018 20:25

bodgit mentioned this pull request May 22, 2018

GSS-TSIG support #549

Closed

miekg closed this Jan 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to use different TSIG algorithms #626

Add ability to use different TSIG algorithms #626

bodgit commented Jan 10, 2018

codecov-io commented Jan 10, 2018 •

edited

Loading

tmthrgd commented Jan 10, 2018

bodgit commented Jan 11, 2018

miekg commented Jan 11, 2018

miekg commented Jan 11, 2018

tmthrgd commented Jan 11, 2018

tmthrgd commented Jan 11, 2018

bodgit commented Jan 11, 2018

bodgit commented Jan 13, 2018

tmthrgd commented Jan 14, 2018

tmthrgd commented Jan 14, 2018 •

edited

Loading

bodgit commented Jan 14, 2018

miekg commented Jan 27, 2018

bodgit commented Feb 27, 2018

miekg commented Feb 28, 2018

bodgit commented Feb 28, 2018

bodgit commented Mar 16, 2018

bodgit commented Jan 4, 2019

miekg commented Jan 4, 2019 via email

bodgit commented Jan 4, 2019

DevKyleS commented Oct 1, 2019 •

edited

Loading

miekg commented Oct 3, 2019 via email

bodgit commented Oct 3, 2019

miekg commented Oct 3, 2019 via email

Add ability to use different TSIG algorithms #626

Add ability to use different TSIG algorithms #626

Conversation

bodgit commented Jan 10, 2018

codecov-io commented Jan 10, 2018 • edited Loading

Codecov Report

tmthrgd commented Jan 10, 2018

bodgit commented Jan 11, 2018

miekg commented Jan 11, 2018

miekg commented Jan 11, 2018

tmthrgd commented Jan 11, 2018

tmthrgd commented Jan 11, 2018

bodgit commented Jan 11, 2018

bodgit commented Jan 13, 2018

tmthrgd commented Jan 14, 2018

tmthrgd commented Jan 14, 2018 • edited Loading

bodgit commented Jan 14, 2018

miekg commented Jan 27, 2018

bodgit commented Feb 27, 2018

miekg commented Feb 28, 2018

bodgit commented Feb 28, 2018

bodgit commented Mar 16, 2018

bodgit commented Jan 4, 2019

miekg commented Jan 4, 2019 via email

bodgit commented Jan 4, 2019

DevKyleS commented Oct 1, 2019 • edited Loading

miekg commented Oct 3, 2019 via email

bodgit commented Oct 3, 2019

miekg commented Oct 3, 2019 via email

codecov-io commented Jan 10, 2018 •

edited

Loading

tmthrgd commented Jan 14, 2018 •

edited

Loading

DevKyleS commented Oct 1, 2019 •

edited

Loading